Skip to content

feat(executor): caller-owned deadline, lockers, and backend PID for concurrent builds - #76

Merged
Kiran01bm merged 3 commits into
mainfrom
kiran01bm/eg4-long-running-build
Sep 4, 2026
Merged

feat(executor): caller-owned deadline, lockers, and backend PID for concurrent builds#76
Kiran01bm merged 3 commits into
mainfrom
kiran01bm/eg4-long-running-build

Conversation

@Kiran01bm

@Kiran01bm Kiran01bm commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

BuildIndexConcurrently gains a caller-owned cancellation mode, the progress tracker becomes the operator's stop path for a running build, and progress snapshots report the lockers a concurrent build is waiting on.

Why

A concurrent index build on a large table can run for hours. Today the only bound the executor accepts is a fixed server-side statement_timeout (ConcurrentBudget.Overall), which fits a synchronous attempt but not an orchestrator that keeps a build alive for as long as it can renew a lease. That orchestrator's decision tree has three branches — my lease lapsed (retry later), an operator stopped it (do not retry), the server's budget killed it (retry with a different bound) — so the executor must report three distinguishable outcomes. Its operators also need to stop a running build without being handed a backend PID they could misuse after the build returns, and to see why a build in the "waiting for old snapshots" phase is not moving, which requires the lockers columns of pg_stat_progress_create_index.

What

  • ConcurrentBudget.CallerOwned: the session runs with statement_timeout = 0 and the caller's cancellable context is the statement's only bound. Overall must be zero (ErrCallerOwnedOverallBudget), and a context that cannot be cancelled is refused with ErrCallerOwnedNeedsCancellableContext before any session is acquired, so the statement remains bounded by construction (LK-2). The bounded mode and every existing caller are unchanged.
  • Cancellation is a three-way partition in both modes: the caller's own context ending is ErrCancelledByCaller (cancelled-by-caller) whichever of the server's 57014 or the client's context error arrives first; a 57014 under a live context is ErrCancelledExternally (cancelled-externally); a 57014 at the bounded mode's deadline is *BudgetError.
  • progress.Tracker.CancelBuild(ctx) signals the active build's backend over the tracker's reserved session, under the same lock that guards the build's lifecycle and only while the build is active (ErrNoActiveBuild otherwise; ErrBuildNotRunning when the backend had no statement to cancel). The tracker never exposes the PID itself.
  • dbconn.ConcurrentIndexProgress reads lockers_total, lockers_done, current_locker_pid; the snapshot carries them as work.lockers_total / work.lockers_done and detail.current_locker_pid. format_version bumps from 2 to 3 and docs/progress-report.md documents the new fields and the stop path.
  • LK-2 in docs/invariants.md records the caller-owned exception; capability, execution-model and TCB docs describe the caller-owned context as a bound different in kind, not an absence of one.
  • Integration tests on a real server: a caller-owned build completes once its blocker releases; a build stopped via tracker.CancelBuild returns ErrCancelledExternally with its invalid leftover reported and the tracker then refuses a second cancel; a build whose caller cancels returns ErrCancelledByCaller, never ErrCancelledExternally or a *BudgetError; a blocked build publishes lockers_total ≥ 1 and the blocker's PID as current_locker_pid.

Before / after

Before
  caller ── ConcurrentBudget{Overall: 30m} ──▶ SET statement_timeout = 30m ──▶ CREATE INDEX CONCURRENTLY
           (no way to say "for as long as my lease holds")
  57014 before the deadline ──▶ ErrCancelledExternally   (operator? caller? indistinguishable)
  tracker.Progress() ──▶ phase, blocks, tuples            (no lockers, no stop path)

After
  caller ── ConcurrentBudget{CallerOwned: true} + cancellable ctx
        ├─ ctx.Done() == nil ──▶ ErrCallerOwnedNeedsCancellableContext (refused)
        └─ SET statement_timeout = 0 ──▶ CREATE INDEX CONCURRENTLY
             ├─ caller's ctx ended    ──▶ ErrCancelledByCaller    (+ catalog verdict)
             ├─ tracker.CancelBuild   ──▶ ErrCancelledExternally  (+ catalog verdict)
             └─ (bounded mode only) deadline ──▶ *BudgetError
  tracker.CancelBuild(ctx) ──▶ pg_cancel_backend on the active build only; PID never leaves the tracker
  tracker.Progress()       ──▶ phase, blocks, tuples, lockers_total/done, current_locker_pid

…oncurrent builds

An orchestrator holding a concurrent index build under a renewable lease
cannot express its bound as a fixed statement_timeout. Let ConcurrentBudget
opt into caller-owned mode, where the cancellable context is the statement's
only bound (a non-cancellable context is refused), expose the build backend
PID through the progress tracker so the build can be cancelled from a second
connection, and add lockers to the progress snapshot so a stalled build
shows what it waits on.
@Kiran01bm
Kiran01bm force-pushed the kiran01bm/eg4-long-running-build branch from 382caea to 5209138 Compare September 4, 2026 04:53
@Kiran01bm
Kiran01bm marked this pull request as ready for review September 4, 2026 04:53
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@aparajon

aparajon commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

🤖 Adversarial correctness review, requested by Armand and performed by his agent. Reviewed at head 52091384.

Verdict: caller-owned mode is bounded by construction and the new refusals hold — but the mode's own primary exit path is the one thing not covered: a build cancelled by its caller can report ErrCancelledExternally, which is exactly the distinction a lease-driven orchestrator needs. Four of five mutations died honestly; the fifth is pinned by a nil-pointer panic rather than its assertion. Nothing blocks, and finding 1 is the one worth an answer before a real orchestrator builds on it.

# Finding Severity
1 Caller-owned 57014 is unconditionally external, even when the caller cancelled medium
2 BuildPID() hands out a PID with no validity window, and its doc prescribes the unsafe use medium
3 The Overall != 0 refusal is pinned only by a nil-pointer panic low (test)
4 PR body says format_version bumps to 2; it bumps to 3 nit

Findings

1. In caller-owned mode every SQLSTATE 57014 becomes ErrCancelledExternally, including the caller's own cancellation. In bounded mode elapsed >= b.Overall was the discriminator between statement_timeout and a third party's pg_cancel_backend. Caller-owned mode has no discriminator, and asConcurrentBudgetError never receives ctx — so it cannot consult the one signal that is free and exact: ctx.Err() != nil means the caller did this. The common path is fine (pgx usually surfaces context.Canceled, which falls through to the operational wrap), but the server's ErrorResponse can beat pgx's local deadline, and on that side of the race the build reports an external cancellation to the very orchestrator that cancelled it. That inverts the motivation in the PR body — an orchestrator "that keeps a build alive for as long as it can renew a lease" cancels its own context when the lease lapses, and would read the outcome as an operator's intervention. None of the three new integration tests exercises it: they cover caller-owned success, pg_cancel_backend via tracker.BuildPID(), and lockers reporting, so the mode's primary exit is untested as well as ambiguous. Two smaller things ride along: the wrapped message says "caller-owned deadline" where the mode's whole point is that there is no deadline, and ErrCancelledExternally's own doc comment still defines itself as "cancelled before its overall budget elapsed", which no longer describes this case.

2. Tracker.BuildPID() returns a bare PID whose safety window the caller cannot observe, and the doc comment prescribes exactly that use. The clearing is right — Start, StartStep, StopConcurrentBuild and Finish all zero buildPID, and TestBuildPIDLifecycle pins three of the four. But a PID a caller has already read stays live in the caller's hand after the build returns and the connection goes back to the pool; pg_cancel_backend on it then cancels whatever the pool next runs on that backend. The window is narrow and, for a library consumer, unmitigable: there is no generation or epoch to re-check against, and the comment reads as an instruction ("An orchestrator passes this PID to pg_cancel_backend from a second connection") with no mention of the boundary.

3. (test) Deleting the Overall != 0 arm of validate leaves TestBuildIndexConcurrentlyRejectsCallerOwnedOverallBudget red for the wrong reason. The mutant fails via pgxpool dereferencing the nil pool at native.go:320, not via the assertion, because require.Error(t, err) accepts any error at all. The sibling one function down does it properly — require.ErrorIs(..., ErrCallerOwnedNeedsCancellableContext) — and dies honestly under the same treatment. (RejectsUnboundedBudget has the same shape on main, so this is precedent rather than a regression.)

4. (nit) The PR body says format_version "bumps to 2"; the bump is 2 → 3. Code, docs/progress-report.md, and both JSON-shape tests agree on 3 — only the body is stale.

Action items

  1. (Finding 1) Pass ctx into asConcurrentBudgetError and, in the CallerOwned arm, return the context error (or a distinct ErrCancelledByCaller) when ctx.Err() != nil, reserving ErrCancelledExternally for a still-live context. Add the missing integration case — caller-owned build, caller cancels its own context, assert the caller-side error and the catalog verdict. While there, drop "deadline" from the caller-owned message and extend ErrCancelledExternally's doc comment to cover the mode.
  2. (Finding 2) Either return (uint32, bool) keyed to a build generation the caller can re-check, or expose Tracker.CancelBuild(ctx) that issues the cancel under the same lock that guards buildPID — the tracker is the only place that knows the build is still live. Failing that, say in the comment that the PID is valid only while the build is active and that a stale one can cancel an unrelated statement on a pooled backend.
  3. (Finding 3) require.ErrorContains(t, err, "Overall to be zero"). Worth the same treatment on RejectsUnboundedBudget while you are in there (optional).
  4. (Finding 4) Fix the "bumps to 2" line in the body.
  5. (optional) TestBuildPIDLifecycle covers StopConcurrentBuild, Finish and Start but not StartStep, which also clears the PID — one more line pins the step-boundary case the comment on StartStep promises.
Verified (tried to break these, couldn't)

Mutation results: deleting the ctx.Done() == nil guard is caught by ErrorIs on ErrCallerOwnedNeedsCancellableContext; deleting the CallerOwned arm of asConcurrentBudgetError is caught, and that arm is load-bearing rather than cosmetic — without it elapsed >= b.Overall with Overall == 0 is always true, so every caller-owned cancellation would have surfaced as a nonsense BudgetError{Budget: 0}; deleting the Overall != 0 arm is caught only incidentally (finding 3). The refusal ordering is right: validate() and the ctx.Done() check both run before any session is acquired, so a misconfigured caller-owned build never touches the pool — ctx.Done() is also correctly nil for the shapes that matter (Background, TODO, WithoutCancel, WithValue over a non-cancellable parent), so the guard is not vacuous. buildPID is cleared on all four transitions, so the stale-PID-from-the-tracker attack dissolves and only the caller-held-PID window in finding 2 survives. The statement_timeout = 0 / lock_timeout = 0 pair keeps the CONCURRENTLY exception intact, and the partial-SET recovery path is untouched. The version bump is consistent everywhere it matters: FormatVersion = 3, the doc's "current version is 3" with per-version provenance, the doc's worked example, and both JSON-shape tests; the format_version: 2 examples in cli-output-examples.md are plan reports on their own contract and correctly left alone, and no second progress example exists to drift. current_locker_pid scans through a *int32 and lands as omitempty, so "omitted when none is published" holds against a 0 from the view. go build ./... clean at head; ./pkg/progress/ green (0.37s) and the non-container ./pkg/executor/ unit tests green. I could not run the three new integration tests locally — no Docker on this machine — so their evidence is CI's: all fourteen checks pass, including test (PostgreSQL 14…18), aws-boundary, and the built-artifact smoke test. No prior review findings to fold in; the Codex reviewer left only a usage-limit notice.

This review was generated by Claude Code (claude-opus-5).

@aparajon

aparajon commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

🤖 Two-lens product review (adoption + integration), same head 52091384. Shorter than the correctness review above and deliberately separate from it — nothing here is a correctness claim.

Lens 1 — OSS adoption ease

The best thing in this PR is a refusal message. ErrCallerOwnedNeedsCancellableContext says what is wrong and why the tool cares — "with statement_timeout disabled the context is the statement's only bound" — which is the difference between a newcomer fixing their call in thirty seconds and filing an issue. More of the error surface should read like that.

current_locker_pid answers the single most common "why is my build stuck?" question, and a standalone user still can't see it. pkg/progress has no consumer outside pkg/executor — no CLI command renders a snapshot — so the progress report is an adapter contract with no first-party reader. Someone who found this repo, ran a concurrent index build against their own database, and watched it sit in "waiting for old snapshots" has the blocker's PID available in a contract they'd have to write Go to reach. That is a fine place to be for a library, but it means this change improves the integration surface rather than first use, and docs/progress-report.md is currently the whole story. If a pg-sprite progress/status surface is on the roadmap, the lockers columns are the strongest argument for it yet.

The capabilities.md positioning sentence got harder to read, not easier. "the safest known online pattern — bounded server timeouts, or an explicit caller-owned cancellable context for concurrent index builds — or refuses with a typed reason" now has two ors doing different jobs in one sentence, and the line runs well past the file's wrap. The one-paragraph pitch is the highest-traffic prose in the repo; the caller-owned exception is probably better as a clause in the bullet below it than as a qualifier in the headline claim. Same for tcb-model.md, where "caller-owned cancellable contexts where a server statement timeout is explicitly disabled" reads as a loophole in "put a limit on everything" rather than as the different-but-equal bound it actually is — worth one clause saying the refusal is what keeps the rule intact.

Lens 2 — SchemaBot integration

The seam is right to design now, because there is no consumer yet. SchemaBot's pkg/engine/postgres imports pkg/dbconn and the plan report, and does not import pkg/progress at all — so BuildPID() and caller-owned mode have zero callers today. That is the cheapest possible moment to fix the two seams in the correctness review: a generation-keyed PID or a CancelBuild method, and a caller-vs-external cancellation distinction. Both become compatibility problems the moment an orchestrator ships against them.

The version bump is correctly scoped and does not gate anything downstream. SchemaBot's strict-version gate is formatVersion != pgplan.FormatVersion — the plan contract — so bumping the progress contract to 3 cannot trip it. The docs' claim that the report contracts move independently holds in practice, not just on paper; no landing-order coupling here.

What integration will actually need next is a reason it can classify, not just an error it can wrap. Caller-owned mode exists for an orchestrator that renews a lease, and that orchestrator's decision tree is: my lease lapsed (retry later, same plan), an operator stopped it (do not retry, tell the human), the server killed it (retry with a different bound). Today the first two collapse into ErrCancelledExternally and the third is a *BudgetError — so two of the three branches are reachable and one is not distinguishable. Typed sentinels are the right shape and no string parsing is needed anywhere, which is the important part; it is the partition that needs one more member.

This review was generated by Claude Code (claude-opus-5).

@aparajon aparajon left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Approving on Armand's behalf after the adversarial correctness review above. The findings there are yours to pick up as follow-ups — flagging them, not gating on them.

This stamp was left by Claude Code (claude-opus-5).

@Kiran01bm

Copy link
Copy Markdown
Collaborator Author

🤖 Adversarial review response — created by Kiran's code review agent (Amp, Claude Opus 4.5) — pull/76, follow-up commit

All five correctness findings and both two-lens prose findings are fixed in the follow-up commit; the progress/status CLI surface is noted as future work, not this PR.

# Finding Status Explanation
1 In caller-owned mode every 57014 became ErrCancelledExternally, including the caller's own cancellation; the mode's primary exit was untested fixed asConcurrentBudgetError now takes the caller's context: ctx.Err() != nil plus a statement cancellation (57014 or a context error) is ErrCancelledByCaller (cancelled-by-caller), in both modes, so whichever of the server's ErrorResponse or pgx's local error wins the race the orchestrator reads its own cancellation. ErrCancelledExternally's doc and message now say "from outside the executor" and the caller-owned wrap no longer says "deadline". New integration test CallerOwnedCallerCancel asserts ErrCancelledByCaller, not-ErrCancelledExternally, not-*BudgetError, and the invalid leftover; TestAsConcurrentBudgetError covers live/ended contexts in both modes.
2 BuildPID() hands out a PID with no validity window, and its doc prescribes the unsafe use fixed BuildPID() is removed. Tracker.CancelBuild(ctx) issues pg_cancel_backend over the reserved verdict session under the tracker's lock, only while the build is active (ErrNoActiveBuild after StopConcurrentBuild/Finish/Start/StartStep) and only against an active backend (ErrBuildNotRunning otherwise), so a stale PID can never reach a pooled backend. Integration test CallerOwnedOperatorCancelViaTracker uses it and asserts the second cancel is refused.
3 RejectsCallerOwnedOverallBudget passes for the wrong reason under the mutant (require.Error accepts the nil-pool panic) fixed New sentinels ErrCallerOwnedOverallBudget and ErrUnboundedBudget; both tests assert with require.ErrorIs, including the optional RejectsUnboundedBudget.
4 PR body says format_version bumps to 2; it bumps to 3 fixed PR body corrected (2 → 3) and refreshed for the CancelBuild / ErrCancelledByCaller changes.
5 TestBuildPIDLifecycle did not pin StartStep clearing the PID fixed TestCancelBuildRefusesOnceTheBuildIsNotActive pins all four transitions — StopConcurrentBuild, Finish, Start, and StartStep.
L1 capabilities.md headline sentence has two ors doing different jobs; tcb-model.md reads the caller-owned context as a loophole fixed Headline now reads "…under bounded sessions, or refuses with a typed reason"; the caller-owned clause moved into the native, as-is bullet with the refusal spelled out. tcb-model.md states the context "stands in for" the statement timeout and that the refusal makes it "different in kind, not absent".
L2 The cancellation partition needs one more member for an orchestrator to classify lease-lapse / operator-stop / server-kill fixed Same change as finding 1: cancelled-by-caller / cancelled-externally / *BudgetError are now three distinct typed outcomes; docs/execution-model.md documents all three.
L3 current_locker_pid is only reachable through Go; no CLI progress surface rejected Out of this PR's scope by design — the seam is being fixed before any consumer exists. A standalone pg-sprite progress/status surface is a fair next step and the lockers columns are its motivation; noted for a follow-up.

Verified correct (refusal ordering before session acquisition, buildPID cleared on all four transitions, statement_timeout = 0 / lock_timeout = 0 pair, FormatVersion = 3 consistency, omitempty on current_locker_pid) — no action.

Source: #76, review comments 5536112496 and 5536112877

@Kiran01bm
Kiran01bm merged commit c5900a0 into main Sep 4, 2026
14 checks passed
Kiran01bm added a commit that referenced this pull request Sep 6, 2026
…on the tracker (#78)

Follow-up to #76: the caller's own cancellation of a concurrent index
build is reported as `cancelled-by-caller` in both modes, and the
operator's stop path becomes `Tracker.CancelBuild` instead of a
handed-out backend PID.

## Why
#76 merged with its caller-owned mode but without the fixes from its
adversarial review. Two of those are correctness gaps an orchestrator
would hit immediately. In caller-owned mode every SQLSTATE 57014 was
read as `ErrCancelledExternally`, including the caller's own
cancellation whenever the server's ErrorResponse arrived before pgx's
local context error — so a lease that lapsed looked like an operator's
intervention, and the orchestrator's retry decision was wrong. And
`Tracker.BuildPID()` handed out a PID with no validity window: a caller
holding a stale one could `pg_cancel_backend` whatever the pool next ran
on that backend.

## What
- `asConcurrentBudgetError` takes the caller's context. A server
cancellation at or past the overall budget is a `*BudgetError` whatever
the caller's context did meanwhile — an orchestrator's deadline commonly
sits just outside the budget it configured, and the escalation signal
must survive that coincidence. Below the budget, an ended caller context
is `ErrCancelledByCaller` (`cancelled-by-caller`) in either mode,
whichever side of the race wins; a 57014 under a live context stays
`ErrCancelledExternally` ("from outside the executor").
`corroborateValidateCancel` gets the same three-way typing, so a
caller-cancelled `VALIDATE CONSTRAINT` is no longer reported as a third
party's cancel. `BudgetError` keeps the server's own error reachable
through `Unwrap`.
- `Tracker.BuildPID()` is removed. `Tracker.CancelBuild(ctx)` issues
`pg_cancel_backend` over the tracker's reserved session, under the
tracker's lock, only while the build is active (`ErrNoActiveBuild` after
`StopConcurrentBuild`/`Finish`/`Start`/`StartStep`). The read of the
backend's state and the signal are one `pg_catalog`-qualified statement
(function and operators included), run detached from the caller's
context on a bounded timeout so a caller deadline cannot tear down the
session the build's verdict needs. A backend the server positively
reports idle is `ErrBuildNotRunning`; a state the server does not expose
(tracking off, hidden from the role) is `ErrBuildUnobservable`, and no
signal is sent blind. A nil return means the signal was sent to a
backend the same statement had just read as active — not that the build
has stopped. The reserved session's role must be able to signal the
backend (same role or `pg_signal_backend`); otherwise
`pg_cancel_backend` raises, and the wrapped error is a permanent
condition of the role, not a retryable one.
- `StopConcurrentBuild` is also deferred, so the PID is retired on every
exit including a panic. It is the fence that keeps `CancelBuild`'s
target the build's own: the executor calls it before the build's session
can return to the pool. `Start`/`StartStep`/`Finish` clear the build
fields as resets under the state lock alone, so the executor's own
updates never wait behind an observation (pinned by
`TestStateMutatorsDoNotWaitForInFlightObservation`). Both the success
and failure verdicts run under their own bounded detached context, so a
build cancelled at the finish line does not report as unproven.
- `Codes()` completeness is pinned by an AST test over the package's
`Code` constants.
- Integration tests on a real server: `CallerOwnedCallerCancel`,
`CallerOwnedOperatorCancelViaTracker`,
`CancelBuildResistsCatalogShadowing` (a `pg_cancel_backend` impostor
ahead of `pg_catalog`), `CancelBuildRefusesAnIdleBackend`; unit tests
pin the budget-over-caller precedence cell, the detached signal context,
and the idle/unobservable partition.
- `docs/execution-model.md` documents the precedence and the three-way
partition; `docs/progress-report.md` documents the stop path and what a
nil return means; `docs/capabilities.md`, `docs/tcb-model.md`,
`docs/invariants.md` (LK-2), `AGENTS.md`, and the review checks stop
calling a caller-owned build "bounded" and name its stop path instead:
the client call is bounded, the server statement runs with
`statement_timeout` off and stops only on a cancel request.

## Before / after
```
Before (#76 as merged)
  caller-owned build, caller's ctx ends
    server 57014 arrives first ──▶ ErrCancelledExternally   (wrong: looks like an operator)
    pgx ctx error arrives first ──▶ context error            (untyped)
  tracker.BuildPID() ──▶ pid  ─ ─ ─ (build returns, pool reuses backend) ─ ─ ▶ pg_cancel_backend(pid)  (hits a stranger)

After
  any concurrent build
    57014 at or past the overall budget       ──▶ *BudgetError            (even if caller's ctx also ended)
    caller's ctx ended, below the budget      ──▶ ErrCancelledByCaller    cancelled-by-caller
    57014, caller's ctx live, below the budget──▶ ErrCancelledExternally  cancelled-externally
  tracker.CancelBuild(ctx)            (one pg_catalog-qualified statement, detached bounded ctx)
    build active, backend 'active'    ──▶ pg_cancel_backend under the tracker's lock; nil = signal sent
    build active, backend idle        ──▶ ErrBuildNotRunning
    build active, state not exposed   ──▶ ErrBuildUnobservable   (no blind signal)
    build not active                  ──▶ ErrNoActiveBuild       (PID never leaves the tracker)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants